src/Aclatraz.performance.test.ts (4)

24-41: Enhance test with more meaningful assertions.

The performance test measures granting permissions 100,000 times but uses a trivial assertion (expect(true).toBe(true)).

  test('grantPermission 100,000 times', () => {
    const rules = [];
    for (let i = 1; i <= 100; i++) {
      rules.push({ id: i, slug: `rule${i}` });
    }
    const acl = new Aclatraz(rules);
    const ruleIds = Array.from({ length: 100 }, (_, i) => i + 1);

+   const permissionResults = [];
    const start = performance.now();
    for (let i = 0; i < 100000; i++) {
-     acl.grantPermission('', ruleIds);
+     permissionResults.push(acl.grantPermission('', ruleIds));
    }
    const end = performance.now();
    console.log(
      `grantPermission 100,000 times took: ${(end - start).toFixed(2)} ms`
    );
-   expect(true).toBe(true);
+   // Verify all permission codes are identical (deterministic)
+   expect(new Set(permissionResults).size).toBe(1);
+   // Verify the permission code is valid
+   expect(permissionResults[0].length).toBeGreaterThan(0);
  });

---

43-61: Improve verify test with meaningful assertions.

The performance test for verification uses a trivial assertion and doesn't validate the actual behavior.

  test('verify (decode) 100,000 times', () => {
    const rules = [];
    for (let i = 1; i <= 100; i++) {
      rules.push({ id: i, slug: `rule${i}` });
    }
    const acl = new Aclatraz(rules);
    const ruleIds = Array.from({ length: 100 }, (_, i) => i + 1);
    const permission = acl.grantPermission('', ruleIds);

+   let verifyResults = [];
    const start = performance.now();
    for (let i = 0; i < 100000; i++) {
-     acl.verify(permission, 50); // Arbitrary ruleId in the middle
+     verifyResults.push(acl.verify(permission, 50)); // Arbitrary ruleId in the middle
    }
    const end = performance.now();
    console.log(
      `verify (decode) 100,000 times took: ${(end - start).toFixed(2)} ms`
    );
-   expect(true).toBe(true);
+   // All verifications should return true since rule 50 is in the permission
+   expect(verifyResults.every(result => result === true)).toBe(true);
+   
+   // Verify that a non-existent rule returns false
+   expect(acl.verify(permission, 101)).toBe(false);
  });

---

63-81: Add meaningful assertions to revokePermission test.

The performance test for revoking permissions doesn't validate the actual behavior.

  test('revokePermission 100,000 times', () => {
    const rules = [];
    for (let i = 1; i <= 100; i++) {
      rules.push({ id: i, slug: `rule${i}` });
    }
    const acl = new Aclatraz(rules);
    const ruleIds = Array.from({ length: 100 }, (_, i) => i + 1);
    const permission = acl.grantPermission('', ruleIds);

+   const finalPermission = permission;
    const start = performance.now();
    for (let i = 0; i < 100000; i++) {
-     acl.revokePermission(permission, [50]); // Arbitrary ruleId
+     const result = acl.revokePermission(permission, [50]); // Arbitrary ruleId
+     if (i === 99999) finalPermission = result;
    }
    const end = performance.now();
    console.log(
      `revokePermission 100,000 times took: ${(end - start).toFixed(2)} ms`
    );
-   expect(true).toBe(true);
+   // Verify rule 50 is revoked in the final permission
+   expect(acl.verify(finalPermission, 50)).toBe(false);
+   
+   // Other rules should still be present
+   expect(acl.verify(finalPermission, 49)).toBe(true);
+   expect(acl.verify(finalPermission, 51)).toBe(true);
  });

---

83-144: Consider using consistent loop structures across tests.

The final three performance tests use very similar structures but slightly different approaches to iteration. For example, the verification test checks every 10,000th rule, but the others don't use sampling.

For better maintainability, consider standardizing the approach across all large-scale tests by:

1. Using consistent sampling methods
2. Adopting similar patterns for creating rule sets
3. Using similar assertion patterns

src/Aclatraz.ts (3)

88-104: Avoid double ‑conversion: decode() → string → BigInt → string again

grantPermission() converts the existing permission from an encoded string
to a padded binary string (decode()), then immediately back to a BigInt
only to stringify it again when calling encode().
This causes two unnecessary string/BigInt round-trips on every call.

A lighter path is to keep the number as BigInt all the way through:

-    let aclNumber = BigInt(`0b${this.decode(currentPermission)}`);
+    // decode directly to BigInt to avoid extra conversions
+    let aclNumber = this.decodeToBigInt(currentPermission);
 ...
-    return this.encode(aclNumber.toString(2), this.options.chunkSize);
+    return this.encodeFromBigInt(aclNumber);

You already have all the building blocks inside encode()/decode(); a pair
of thin wrappers (decodeToBigInt, encodeFromBigInt) would make the core
logic faster (no string copies) and easier to read.

---

111-127: Symmetric improvements recommended for revokePermission()

The same conversion chain discussed above is repeated here.
Once helper methods that operate directly on BigInt exist,
revokePermission() can mirror the streamlined flow and avoid the
current overhead.

---

169-175: padStart may add superfluous leading zeros

After assembling the full aclNumber, you pad it to
chunkSize * chunks.length.
If the most-significant chunk is not fully utilised (very common), this
adds extra zeros that will later be re-encoded, lengthening the permission
string unnecessarily.

A safer variant is to trimStart() after padding or compute the minimal
bit-length needed.

This keeps encodings compact without losing information.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

• package.json: Language not supported